Skip to content

Conversation

dineshSajwan
Copy link
Contributor

@dineshSajwan dineshSajwan commented Sep 29, 2025

Issue # (if applicable)

Related to aws/aws-cdk-rfcs#785

Reason for this change

Adding bedrock agent core runtime and runtime endpoint

Description of changes

  • Added a new L2 construct for runtime in aws -bedrock-agentcore-alpha package.
  • Added a new L2 construct for runtime endpoint
  • Added test cases
  • Added documentation

Describe any new or updated permissions being added

The runtime creates a role with permission to ecr repo, cloudwatch , xray .

Description of how you validated changes

Unit tests, integration tests, manual tests

Checklist


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@github-actions github-actions bot added p2 repeat-contributor [Pilot] contributed between 3-5 PRs to the CDK labels Sep 29, 2025
@aws-cdk-automation aws-cdk-automation requested a review from a team September 29, 2025 22:32
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(This review is outdated)

@dineshSajwan dineshSajwan changed the title feat(agentcore): add agentcore runtime L2 constructs feat(agentcore): add agentcore runtime L2 construct Sep 29, 2025
@aws-cdk-automation aws-cdk-automation dismissed their stale review September 30, 2025 02:43

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

@alvazjor alvazjor self-assigned this Sep 30, 2025
/**
* Agent runtime artifact configuration for Bedrock Agent Core Runtime
*/
export interface AgentRuntimeArtifactConfig {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we expecting this configuration to have more elements in the future? Do you know if there are plans for that? Seems like having this interface at the moment is not adding too much value, you could potentially return the URI directly in the bind method rather than this object (in CDK, we tend to favor flat composition over nested composition if we can)

Copy link
Contributor Author

@dineshSajwan dineshSajwan Oct 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This mirrors the cloudformation structure of agentruntimeartifact . IMHO, we should keep it as cloud formation structure because if the underlying service add any new field in this object then the flat structure of containeruri may cause breaking changes for the L2 construct.

* @param clientId Cognito App Client ID
* @param region Optional AWS region (defaults to stack region)
*/
public configureCognitoAuth(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This and the other methods below are exactly what I was expecting to see instead of https://github.com/aws/aws-cdk/pull/35623/files#r2398909386.
Simple and specific for each type of auth configuration. I would prefer to keep only this idea and drop the possibility for the user to set up manually the auth config with all the possible wrong combinations they could make if we let it fully configurable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack.

public readonly endpointName: string;
public readonly agentRuntimeArn: string;
public readonly status?: string;
public readonly targetVersion?: string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the difference between this and the agentRuntimeVersion ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We actually have two of those liveVersion and targetVersion. I have added liveVersion with this new commit.

this.liveVersion = this.endpointResource.attrLiveVersion;
this.targetVersion = this.endpointResource.attrTargetVersion;

https://docs.aws.amazon.com/AWSCloudFormation/latest/TemplateReference/aws-resource-bedrockagentcore-runtimeendpoint.html

https://docs.aws.amazon.com/bedrock-agentcore/latest/devguide/agent-runtime-versioning.html

As per my understanding

Initial Creation :

  • You set agentRuntimeVersion: "1"
  • Both liveVersion and targetVersion will be "1"
  • Status: READY

During Update :

  • Update runtime version to "2"
  • liveVersion remains "1"
  • targetVersion becomes "2" (transitioning to this)
  • Status: UPDATING

After Update Completes :

  • Both liveVersion and targetVersion become "2"
  • Status: READY

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont mind having this if CFN also provides it, but I also dont get how this is useful. CDK does not allow interaction "during updates'. In CDK, this will generate a reference to a value that is actually available only after this resource is deployed, and by that time, both liveVersion and targetVersion will be have the same value. Again, I dont mind having it, since CFN is providing it, but lets make the documentation as clear as possible about the differences of both fields

@mergify mergify bot dismissed alvazjor’s stale review October 6, 2025 17:01

Pull request has been modified.

@dineshSajwan dineshSajwan marked this pull request as ready for review October 6, 2025 19:40
*
* @returns RuntimeAuthorizerConfiguration for IAM authentication
*/
public static configureIAM(): RuntimeAuthorizerConfiguration {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename to: usingIAM

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack.

* @param allowedAudience Optional array of allowed audiences
* @returns RuntimeAuthorizerConfiguration for JWT authentication
*/
public static configureJWT(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename to usingJWT

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack.

* @param region Optional AWS region where the User Pool is located (defaults to stack region)
* @returns RuntimeAuthorizerConfiguration for Cognito authentication
*/
public static configureCognito(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename to usingCognito

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack.

* @param scopes Optional array of OAuth scopes
* @returns RuntimeAuthorizerConfiguration for OAuth authentication
*/
public static configureOAuth(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename to usingOAuth

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack.

provider: string,
discoveryUrl: string,
clientId: string,
scopes?: string[],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

provider and scopes parameters are ignored in the constructor if the OAuthAuthorizerConfiguration class. Why do we need them here then?

Copy link
Contributor Author

@dineshSajwan dineshSajwan Oct 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently OAuthAuthorizerConfiguration and JwtAuthorizerConfiguration are behaving same way under the hood . In my view usingOAuth provides a better user experience (having a dedicated usingOAuth method makes it clear that developers are configuring OAuth providers like GitHub or Google, even though the underlying AWS service treats it the same as JWT authentication). I kept provider and scopes for potential future enhancements where we might add OAuth-specific features or some metadata for observability.
I'm happy to remove these unused parameters to keep the code cleaner. Would you prefer I remove them, or keep them documented as reserved for future use?

Copy link
Contributor

@alvazjor alvazjor Oct 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets remove them for now, since they are not used. If we add features that need then, we can introduce them later. In the future, if more specific configuration is required for each OAuth providers, we can introduce special methods, like usingGoogleOAuth or something like that, and in there we request the additional data. From a user perspective, if the name of the method already gives you the provider, is very clear what is the intention behind it, and with the current approach we can manage to introduce this later when needed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack.

* Optional description for the agent runtime endpoint
* @default - No description
*/
readonly description?: string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CFN has a pattern for this field, lets add it here too please (JSDoc)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack

* A list of key:value pairs of tags to apply to this RuntimeEndpoint resource
* @default {} - no tags
*/
readonly tags?: { [key: string]: string };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CFN has a pattern for this field, lets add it here too please (JSDoc)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack

value: this.description,
fieldName: 'Description',
minLength: 1,
maxLength: 1200,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In CFN doc, the max length is 256

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh they changed it. Will update thanks.

const account = Stack.of(this).account;

// ECR Image Access - scoped to account repositories
role.addToPolicy(new iam.PolicyStatement({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This policy is needed if the runtime artifact is fromEcrRepository right? Could we check if the agentRuntimeArtifact is of type EcrImage to add this policy? And maybe even restrict the resource to that specific one?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was following these permission as per this . However , I realized that ECR permissions are redundant for L2 construct because these are already handled with AgentRuntimeArtifact bind function with grantPull. The ecr grantPull take care of permissions in best possible way. We need these permission during bind so that it can pull the image from ecr repository before deploying it on runtime. I have removed these permission from createExecutionRole and have tested the changes.

}));

// ECR Token Access - must be * for authorization token
role.addToPolicy(new iam.PolicyStatement({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as the previous policy, if only needed when artifact is fromEcrRepository, we should try and restrict the creation only in that case

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above , removed the permission as they were redundant and using ecr grantPull to grant the permission.

@mergify mergify bot dismissed alvazjor’s stale review October 9, 2025 17:02

Pull request has been modified.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

p2 repeat-contributor [Pilot] contributed between 3-5 PRs to the CDK

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants